Since a subtree is a Merkle tree, this calculates a Merkle Tree Hash. I think this function is equivalent to tlog.TreeHash and we can reuse that, right?

I note that this function takes a list of leaves in-memory, while tlog.TreeHash takes a HashReader, which generalizes over hash storage. But if we want to use leaves in-memory, we can pass a HashReaderFunc that accesses them.

So, I think we can do without this function.

HashLeaves, renamed from Hash to make its intention clearer, has just one external caller that needs it and it requires this shape. For each uploaded package, the tlog-mirror-srv's add-entries handler hashes the received leaf entries with tlog.RecordHash, combines those leaf hashes into the subtree hash with HashLeaves, and verifies the package's subtree consistency proof against the pending checkpoint's root. At that point the entries are not in any tree and thus not loaded into a reader.

That said, it doesn't necessarily have to live here, this just seemed like the best home for it.

I think this can also be implemented in terms of tlog.TreeHash(hi-lo, someReader).

The distinction is that TreeHash will make storage calls starting from zero, but we need to make sure our storage layer is correctly indexing based on the subtree we want. I think we can achieve that with a tlog.HashReaderFunc:

func subtreeHash(lo, hi int64, reader xtlog.HashReader) (xtlog.Hash, error) {
  return tlog.TreeHash(hi-lo, func(zeroBasedIndexes []int64) []Hash {
    var absoluteIndexes []int64
    for _, i := range zeroBasedIndexes {
      absoluteIndexes = append(absoluteIndexes, i+lo)
    }
    return reader.ReadHashes(absoluteIndexes)
  }
}

Using the HashReaderFunc is very clever, but the way you're calculating the offset is subtly wrong. The indexes TreeHash hands our reader aren't leaf numbers. They're stored hash indexes, a flat numbering of every node in the tree, with leaves and internal nodes interleaved. So i + lo is adding a leaf count to something that isn't a leaf position. The offset has to account for the node's level. A leaf may be lo away from another leaf, but a node spanning 8 leaves moves by only lo / 8.

So instead of offsetting the index, we decode it, shift its position by lo scaled to that level, and re-encode:

level, n := tlog.SplitStoredHashIndex(idx)
absolute := tlog.StoredHashIndex(level, (lo>>level)+n)

Which gives us:

func rangeHash(lo, hi int64, reader tlog.HashReader) (tlog.Hash, error) {
	return tlog.TreeHash(hi-lo, tlog.HashReaderFunc(
		func(zeroBasedIndexes []int64) ([]tlog.Hash, error) {
			absoluteIndexes := make([]int64, len(zeroBasedIndexes))
			for i, idx := range zeroBasedIndexes {
				level, n := tlog.SplitStoredHashIndex(idx)
				absoluteIndexes[i] = tlog.StoredHashIndex(level, (lo>>level)+n)
			}
			return reader.ReadHashes(absoluteIndexes)
		}))
}

But fixing the offset surfaces our next problem. lo>>level is only exact when lo is a multiple of 2^level, which holds for every range we currently pass to rangeHash(). If a later change passes a lo that isn't, the shift drops bits, the index points to a different node, and we silently fold a wrong hash.

So we would need to add a guard:

func rangeHash(lo, hi int64, reader tlog.HashReader) (tlog.Hash, error) {
	return tlog.TreeHash(hi-lo, tlog.HashReaderFunc(
		func(zeroBasedIndexes []int64) ([]tlog.Hash, error) {
			absoluteIndexes := make([]int64, len(zeroBasedIndexes))
			for i, idx := range zeroBasedIndexes {
				level, n := tlog.SplitStoredHashIndex(idx)
				loAtLevel := lo >> level
				if loAtLevel<<level != lo {
					return nil, fmt.Errorf("subtreeHash: lo=%d is not a multiple of 2^%d", lo, level)
				}
				absoluteIndexes[i] = tlog.StoredHashIndex(level, loAtLevel+n)
			}
			return reader.ReadHashes(absoluteIndexes)
		}))
}

For what it's worth, your idea does work. That said, the version we have doesn't require this awkward translation layer that's somewhat difficult to reason about.

This API shape is surprising to me. Being willing to hash an arbitrary set of leaves a) feels like something to be left for a general package (like xtlog itself), not subtree; and b) feels dangerous! What if the caller doesn't respect the contract that they need to make sure it's a valid subtree first?

I think this package should only expose func SubtreeHash(start, end int64, reader tlog.HashReader) tlog.Hash. It can verify that the interval is a valid subtree, then read all the necessary hashes from the reader itself. This guarantees that we never supply an invalid set of input hashes.

I think the existing API of this function might be acceptable as a private helper, but not as part of the public interface of the package.

HashLeaves, renamed from Hash to make its intention clearer, has just one external caller that needs it and it requires this shape. For each uploaded package, the tlog-mirror-srv's add-entries handler hashes the received leaf entries with tlog.RecordHash, combines those leaf hashes into the subtree hash with HashLeaves, and verifies the package's subtree consistency proof against the pending checkpoint's root. At that point the entries are not in any tree and thus not loaded into a reader.

That said, it doesn't necessarily have to live here, this just seemed like the best home for it.

This feels too clever, as though you're optimizing for a case we don't actually have (and don't want to support). Even if we do want to compute hashes of arbitrary intervals (which as I note above, I don't think we do -- I'm pretty sure all of our inputs to this function are valid subtrees), the MTC draft shows that any arbitrary interval can be decomposed into two subtrees: a perfect left subtree, and a ragged right subtree.

This function's approach is to do a big recursive "find all the indices", followed by a second recursive "combine the hashes". But the whole thing can be done in a single, much-more-simply recursive "look up hash of left perfect tree, combine with recursive computation of hash of right ragged tree".

You're right, all of the inputs to this function are valid subtrees. But this isn't meant to support arbitrary intervals. tlog.HashReader is attached to our tile store (S3/MinIO) and each ReadHashes call is a round trip. A single recursion would read one hash per subtree resulting in about log N round trips. Gathering the indices up front lets us do it in a single batched read, which is the only reason we build the list of indices first.